Make router health checks concurrent, bounded, and non-blocking#491
Conversation
haiyanmeng
left a comment
haiyanmeng
left a comment
There was a problem hiding this comment.
Yiqing Wang (@YQ-Wang) , thanks for the great work!
Just one comment, should we consider running checkEnvoy, checkK8s, and checkAteAPI concurrently? This could limit the time of each check call to ~500ms instead of 1500ms.
|
Just curious: should we add liveness/readiness for router itself? |
|
haiyanmeng Good suggestion, thank you. I updated the three dependency checks to run concurrently, so each health cycle is now bounded by the slowest check (about 500 ms). I also added a regression test for this. |
|
Haven Xia (@HavenXia) Good point. Dedicated router liveness and readiness probes would be useful, but I think they are better handled in a follow-up with lightweight /livez and /readyz endpoints and clearly defined semantics, rather than reusing /statusz. |
haiyanmeng
left a comment
haiyanmeng
left a comment
There was a problem hiding this comment.
Thanks for making the checks concurrent.
One tiny comment, otherwise looks good to me.
| if !envoyResult.healthy { | ||
| slog.ErrorContext(ctx, "Envoy health check failed", slog.String("msg", envoyResult.message)) | ||
| } | ||
| if !k8sResult.healthy { | ||
| slog.ErrorContext(ctx, "Kubernetes API health check failed", slog.String("msg", k8sResult.message)) | ||
| } | ||
| if !ateResult.healthy { | ||
| slog.ErrorContext(ctx, "ATE API gRPC health check failed", slog.String("msg", ateResult.message)) | ||
| } |
There was a problem hiding this comment.
Can we move this logic to runComponentHealthCheck? It would look like:
func runComponentHealthCheck(ctx context.Context, failMsg string, check func(context.Context) (bool, string)) componentHealthCheckResult {
healthy, message := check(ctx)
if !healthy {
slog.ErrorContext(ctx, failMsg, slog.String("msg", message))
}
return componentHealthCheckResult{
healthy: healthy,
message: message,
checkedAt: time.Now(),
}
}
|
Yiqing Wang (@YQ-Wang) , can you update the PR title and description to make it clear that it also makes the checks concurrent? Thanks! |
Yiqing Wang (YQ-Wang)
changed the title
haiyanmeng done, thanks! |
|
Help merge before you have membership propagated :D |
4 participants
Summary
/versionhealth check observe the router context and a 500 ms dependency timeout/statuszto serve the most recently completed reportRoot cause
The client-go
Discovery().ServerVersion()helper usescontext.TODO()internally. If the Kubernetes API accepted/versionbut withheld its response, canceling the router context could not terminate the request. The router also held the report mutex across dependency network calls, so the stalled request blocked/statuszand delayed shutdown.The three dependency checks also ran sequentially. Even with a 500 ms timeout per dependency, one health cycle could therefore take up to about 1.5 seconds. Running them concurrently bounds the cycle by the slowest check, about 500 ms.
Validation
make verifygo test ./cmd/atenet/internal/router -run 'Test(CheckK8s|HealthCheck|HealthStart)' -count=20go test -race ./cmd/atenet/internal/router -count=1/versionresponse and withheld it;/statuszremained responsive with HTTP 200, the request was canceled at the 500 ms deadline, and one Kubernetes health failure was recordedFixes #490